Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom modal aux click #6891

Merged
merged 6 commits into from
Aug 12, 2024
Merged

Conversation

hoonji
Copy link
Member

@hoonji hoonji commented Aug 12, 2024

Motivation for features / changes

On some browsers, the custom modal close logic will be triggered immediately after releasing the right click button, which causes the modal to be closed as soon as it is opened.

Technical description of changes

The modal close event handler will ignore the auxclick (right click) mouseup event. Modals can still be closed with left clicks and the contextmenu event.

Screenshots of UI changes (or N/A)

Status quo: image

@hoonji hoonji requested a review from arcra August 12, 2024 05:49
.pipe(
skipUntil(timer(250)) // prevent closing immediately after modal open.
)
.subscribe((event) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, the previous version of the code was executing immediately in some browsers (but apparently not in all of them), causing the event handler to be attached right before the "mouse up" event, or something like this, causing it to close it.

Can we instead check the type of event? Or something like this, to only match clicks? If an alternative solution is more complex, or difficult to validate, or if what I'm saying is not accurate or something, I'm ok with this solution. Just thought I'd ask.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is better! I hadn't realized that there are two separate events for "right clicking": contextmenu (immediately triggers the system context menu) and auxclick (triggers when mousedown and mouseup occurs in the same element)

Since we're using contextmenu to open the modal, I updated the code to simply ignore the auxclick events, which should have the effect of ignoring the right click mouseup. To close the modal, left clicks and contextmenu events can still be used.

@hoonji hoonji merged commit 2d1bf5e into tensorflow:master Aug 12, 2024
13 checks passed
groszewn pushed a commit to groszewn/tensorboard that referenced this pull request Aug 13, 2024
## Motivation for features / changes
On some browsers, the [custom modal
close](https://github.com/tensorflow/tensorboard/blob/0627ad5480b309a5f3f69f39e52670496c2e5863/tensorboard/webapp/widgets/custom_modal/custom_modal.ts#L116)
logic will be triggered immediately after releasing the right click
button, which causes the modal to be closed as soon as it is opened.

## Technical description of changes
The modal close event handler will ignore the `auxclick` (right click)
mouseup event. Modals can still be closed with left clicks and the
`contextmenu` event.

## Screenshots of UI changes (or N/A)
Status quo:
![image](https://github.com/user-attachments/assets/5b4c0c8f-4878-4cbe-a46a-cd425c188061)
groszewn pushed a commit that referenced this pull request Aug 13, 2024
## Motivation for features / changes
On some browsers, the [custom modal
close](https://github.com/tensorflow/tensorboard/blob/0627ad5480b309a5f3f69f39e52670496c2e5863/tensorboard/webapp/widgets/custom_modal/custom_modal.ts#L116)
logic will be triggered immediately after releasing the right click
button, which causes the modal to be closed as soon as it is opened.

## Technical description of changes
The modal close event handler will ignore the `auxclick` (right click)
mouseup event. Modals can still be closed with left clicks and the
`contextmenu` event.

## Screenshots of UI changes (or N/A)
Status quo:
![image](https://github.com/user-attachments/assets/5b4c0c8f-4878-4cbe-a46a-cd425c188061)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants